Support loading plugins from multiple directories #7871

priyen · 2021-12-06T17:12:16Z

Description

At Stripe, one of our use cases requires us to see if we can load plugins from more than 1 directory. This change allows Pinot to load plugins from directories other than just the root/default one. The existing plugin java property name, PLUGINS_DIR_PROPERTY_NAME = "plugins.dir", originally is a string of 1 path, but now is a semi-colon delimited string which can have multiple paths. PLUGINS_INCLUDE_PROPERTY_NAME = "plugins.include" was also updated to be semi-colon delimited to be consistent.

Github issue: #7875

Testing performed:

existing automated tests
added a new test for the additional tar gz function that can tar multiple files
manual testing: started a pinot cluster with plugins.dir being set to string with 2 directories: ie; "/etc/plugins1;/etc/plugins2" and from logs can confirm plugins were found and loaded from both. Also confirmed the plugins.include was respected and only plugins inside this property would be loaded.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

no it does not

Does this PR fix a zero-downtime upgrade introduced earlier?

n/a

Does this PR otherwise need attention when creating release notes? Things to consider:
New configuration options:

update documentation to show that the plugin dir property can be a semi-colon delimited string of multiple paths

Deprecation of configurations: n/a

Signature changes to public methods/interfaces:

#getPluginsRootDir in PluginManager.java -> #getPluginsDirectories, I've replaced all uses of this as part of this PR
the addition of #createTarGzFile which accepts multiple files instead of just 1 file
New plugins added or old plugins removed
n/a

Release Notes

Pinot can now load plugins from multiple directories:

PLUGINS_DIR_PROPERTY_NAME = "plugins.dir", originally is a string of 1 path, but now is a semi-colon delimited string of multiple paths. For example: /dir1/;/dir2
PLUGINS_INCLUDE_PROPERTY_NAME = "plugins.include" is also a semi-colon delimited string of multiple plugins. For example: plugin1;plugin2

Documentation

Looking for an optimal place to add this new info, but I don't really see it documented anywhere: https://github.com/pinot-contrib/pinot-docs/search?q=plugins.dir

Thoughts?

codecov-commenter · 2021-12-06T21:44:09Z

Codecov Report

Merging #7871 (04ec583) into master (e572ea0) will decrease coverage by 0.27%.
The diff coverage is 51.85%.

@@             Coverage Diff              @@
##             master    #7871      +/-   ##
============================================
- Coverage     71.65%   71.38%   -0.28%     
- Complexity     4080     4082       +2     
============================================
  Files          1581     1583       +2     
  Lines         81350    81885     +535     
  Branches      12128    12242     +114     
============================================
+ Hits          58293    58452     +159     
- Misses        19117    19475     +358     
- Partials       3940     3958      +18

Flag	Coverage Δ
integration1	`29.16% <12.96%> (-0.15%)`	⬇️
integration2	`27.70% <12.96%> (-0.11%)`	⬇️
unittests1	`68.48% <51.85%> (-0.33%)`	⬇️
unittests2	`14.41% <0.00%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ava/org/apache/pinot/spi/plugin/PluginManager.java	`55.41% <44.68%> (+6.86%)`	⬆️
...ache/pinot/common/utils/TarGzCompressionUtils.java	`85.91% <100.00%> (+0.62%)`	⬆️
...m/function/JsonExtractScalarTransformFunction.java	`22.49% <0.00%> (-27.04%)`	⬇️
...a/manager/realtime/RealtimeSegmentDataManager.java	`50.00% <0.00%> (-25.00%)`	⬇️
...ransform/function/IdentifierTransformFunction.java	`69.76% <0.00%> (-8.50%)`	⬇️
...nction/DistinctCountBitmapAggregationFunction.java	`49.74% <0.00%> (-7.78%)`	⬇️
...a/org/apache/pinot/core/common/DataBlockCache.java	`91.44% <0.00%> (-4.77%)`	⬇️
...r/helix/SegmentOnlineOfflineStateModelFactory.java	`58.49% <0.00%> (-4.72%)`	⬇️
...java/org/apache/pinot/core/common/DataFetcher.java	`85.71% <0.00%> (-4.25%)`	⬇️
...t/local/upsert/PartitionUpsertMetadataManager.java	`76.23% <0.00%> (-3.98%)`	⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e572ea0...04ec583. Read the comment docs.

priyen · 2021-12-07T15:44:53Z

@Jackie-Jiang @jadami10 Would you review this when you get a chance? Thanks!

...in/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java

jadami10 · 2021-12-07T16:44:43Z

...in/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java

+    File pluginsTarGzFile = new File(PINOT_PLUGINS_TAR_GZ);
+    try {
+      File[] files = validPluginDirectories.toArray(new File[0]);
+      TarGzCompressionUtils.createTarGzFile(files, pluginsTarGzFile);


how does this untar later? If I have

/a/b /a/c /b/c

will it come back out the same way?

so the initial createTarGzFile which accepted 1 File obj, also accepted directories with support for recursion too: so let's say I called createTarGzFile with 1 directory (/a/...) that entire one will be tarred and it's children.

now, using the new method if I call it on let's say [ /a/ and /b/ ], the dir name is used as the baseEntryName (see ln 89 in TarGzCompressionUtils.java).

so effectively, if you tar two directories /a/ and /b/, it should come the same way as you pasted above

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java

jackjlli · 2021-12-07T19:00:38Z

pinot-common/src/test/java/org/apache/pinot/common/utils/TarGzCompressionUtilsTest.java

+    assertEquals(untarredFiles.size(), 4);
+
+    File untarredFileDir1 = untarredFiles.get(0);
+    File untarredFileDir2 = untarredFiles.get(2);


Please add some comments on why the 1st and 3rd would be fetched.

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

pinot-tools/src/main/resources/appAssemblerScriptTemplate

pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java

Jackie-Jiang · 2021-12-09T00:23:43Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

+  @VisibleForTesting
+  public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+      IllegalArgumentException {
+    String[] directories = pluginsDirectories.split(";");


I feel it is more common to use , as the array separator (e.g. in Apache commons configuration). Is there some special reason why picking ; as the separator here?

(minor) Use StringUtils.split(pluginsDirectories, ',') for slightly better performance (avoid regex checking)

I used ; as the plugin to include property also used it to be consistent, but I am fine with changing it to ,, what do you think?

+1 to priyen. plugins themselves use ;. and most PATH-like things use ; as well?

I just realized that the pluginsInclude separator is changed from ',' to ';' in this PR which can cause backward incompatibility. Does it make sense to allow both as separator?

This is a bit odd, the master bash code in pinot-tools/src/main/resources/appAssemblerScriptTemplate is using export IFS=";" when looping through $PLUGINS_INCLUDE ..now I'm wondering if pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java ln 157 pluginsToLoad = Arrays.asList(pluginsInclude.split(",")); in master was even working in the first place..

@xiangfu0 Can you please take a look?

friendly ping, any ideas?

Right now Pinot uses appAssemblerScriptTemplate to set all the plugins into java classpath.
So far let's use semi-colon to make the delimiter.

For PluginManager.java, we should also follow the same delimiter convention to use semi-colon. For pluginInclude, we can make the colon as backward compatible.

jadami10

thanks for working on the feedback! this looks good!

...in/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentGenerationJobRunner.java

jadami10 · 2021-12-09T00:53:42Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

+  @VisibleForTesting
+  public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+      IllegalArgumentException {
+    String[] directories = pluginsDirectories.split(";");


+1 to priyen. plugins themselves use ;. and most PATH-like things use ; as well?

jadami10 · 2021-12-09T00:56:08Z

pinot-spi/src/test/java/org/apache/pinot/spi/plugin/PluginManagerTest.java

-    _jarDirFile = new File(jarDir);
-    _jarDirFile.mkdirs();
+  @Test
+  public void testGetPluginsToLoad()


solid test!

xiangfu0 · 2021-12-08T23:38:48Z

pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java

-        BufferedOutputStream bufferedOut = new BufferedOutputStream(fileOut);
-        OutputStream gzipOut = new GzipCompressorOutputStream(bufferedOut);
-        TarArchiveOutputStream tarGzOut = new TarArchiveOutputStream(gzipOut)) {
+         BufferedOutputStream bufferedOut = new BufferedOutputStream(fileOut);


fix the indentation?

xiangfu0 · 2021-12-12T02:27:42Z

pinot-common/src/main/java/org/apache/pinot/common/utils/TarGzCompressionUtils.java

+   * Creates a tar.gz file from a list of input file/directories to the output file. The output file must have
+   * ".tar.gz" as the file extension.
+   */
+  public static void createTarGzFile(File[] inputFiles, File outputFile)


shall we check and skip the nested paths?
E.g. one directory is a/b/c and one file a/b/c/d.file

@xiangfu0 can you clarify what you mean, so a/b/c/d.file would be skipped since it's included already via a/b/c?

yes, if the override happens then it's not a problem to worry.

xiangfu0 · 2021-12-12T02:57:00Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

+  @VisibleForTesting
+  public HashMap<String, File> getPluginsToLoad(String pluginsDirectories, String pluginsInclude) throws
+      IllegalArgumentException {
+    String[] directories = pluginsDirectories.split(";");


Right now Pinot uses appAssemblerScriptTemplate to set all the plugins into java classpath.
So far let's use semi-colon to make the delimiter.

For PluginManager.java, we should also follow the same delimiter convention to use semi-colon. For pluginInclude, we can make the colon as backward compatible.

priyen changed the title ~~Support loading plugins from directories~~ Support loading plugins from multiple directories Dec 6, 2021

Support multiple plugin directories

4acfecb

priyen force-pushed the support-multiple-plugin-directories branch from 6b3db36 to 4acfecb Compare December 6, 2021 19:38

priyen-stripe added 3 commits December 6, 2021 15:05

Clarify comment

1db47ec

update appAssemblerScriptTemplate to add multiple plugin dirs

2722e27

Fix list/arr conversion

f32149c

priyen-stripe added 2 commits December 6, 2021 17:13

Add testDirectories test in TarGzCompressionUtilsTest

166d330

Fix bash script IFS unset

197bd5e

priyen force-pushed the support-multiple-plugin-directories branch from aeae4df to 197bd5e Compare December 7, 2021 07:38

priyen-stripe added 2 commits December 7, 2021 02:53

Don't check for both null and empty

cef7a1f

Fix conversion error

541911a

priyen marked this pull request as ready for review December 7, 2021 14:50

priyen marked this pull request as draft December 7, 2021 14:55

priyen marked this pull request as ready for review December 7, 2021 15:41

priyen mentioned this pull request Dec 7, 2021

Support loading plugins from multiple directories #7875

Open

jadami10 reviewed Dec 7, 2021

View reviewed changes

jackjlli reviewed Dec 7, 2021

View reviewed changes

priyen-stripe added 2 commits December 8, 2021 18:29

Addr review comments + test for multiple plugin loading

6c4365b

Adding empty tail line

04ec583

xiangfu0 requested review from xiangfu0 and Jackie-Jiang December 8, 2021 23:36

Jackie-Jiang reviewed Dec 9, 2021

View reviewed changes

priyen requested a review from Jackie-Jiang December 9, 2021 00:48

jadami10 approved these changes Dec 9, 2021

View reviewed changes

Format code using style guide

471591e

priyen force-pushed the support-multiple-plugin-directories branch from 4bc5353 to 471591e Compare December 9, 2021 00:58

xiangfu0 reviewed Dec 14, 2021

View reviewed changes

xiangfu0 approved these changes Dec 14, 2021

View reviewed changes

xiangfu0 added 0.10.0 release-notes Referenced by PRs that need attention when compiling the next release notes labels Dec 14, 2021

xiangfu0 merged commit 9127286 into apache:master Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support loading plugins from multiple directories #7871

Support loading plugins from multiple directories #7871

priyen commented Dec 6, 2021 •

edited

Loading

codecov-commenter commented Dec 6, 2021 •

edited

Loading

priyen commented Dec 7, 2021

jadami10 Dec 7, 2021

priyen Dec 9, 2021

jackjlli Dec 7, 2021

Jackie-Jiang Dec 9, 2021

priyen Dec 9, 2021 •

edited

Loading

jadami10 Dec 9, 2021

Jackie-Jiang Dec 9, 2021

priyen Dec 9, 2021 •

edited

Loading

Jackie-Jiang Dec 9, 2021

priyen Dec 10, 2021

xiangfu0 Dec 12, 2021

jadami10 left a comment

jadami10 Dec 9, 2021

jadami10 Dec 9, 2021

xiangfu0 Dec 8, 2021

xiangfu0 Dec 12, 2021

priyen Dec 14, 2021

xiangfu0 Dec 14, 2021

xiangfu0 Dec 12, 2021

Support loading plugins from multiple directories #7871

Support loading plugins from multiple directories #7871

Conversation

priyen commented Dec 6, 2021 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

codecov-commenter commented Dec 6, 2021 • edited Loading

Codecov Report

priyen commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyen Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyen Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadami10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyen commented Dec 6, 2021 •

edited

Loading

codecov-commenter commented Dec 6, 2021 •

edited

Loading

priyen Dec 9, 2021 •

edited

Loading

priyen Dec 9, 2021 •

edited

Loading